Vite: Use vite hook filter for performance improvements#34022
Vite: Use vite hook filter for performance improvements#34022huang-julien wants to merge 9 commits intonextfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors multiple Vite plugin hooks from function-based signatures to structured objects exposing filter and handler (or resolver filter/handler), preserving existing logic and observable outputs across builder and framework plugins. Public plugin signatures unchanged; transform/resolveId internals now use { filter, handler } shapes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 01380b7
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/frameworks/vue3-vite/src/plugins/vue-component-meta.ts`:
- Around line 126-132: The guard that tries to detect re-exports incorrectly
matches local exports because it uses unescaped name in loose regexes; update
the check in vue-component-meta.ts that uses the variables name and src so it
only treats true re-exports as re-exports: first escape name with a proper
escapeRegExp helper before composing regexes, and change the first pattern to
require a from clause (e.g. only match "export { ... } from ...") instead of
matching any "export { ... }" so local "export { Foo }" is not treated as a
re-export; also avoid using \b around name (or only apply \b when name matches
JS identifier characters) so names with non-word chars like "$Foo" are handled
correctly. Ensure the modified checks preserve the other re-export branch
(export \* from ...) and still fall back to !src.includes(name) where
appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53a0d31a-a051-4e3c-8e5e-5b020a5c4f0c
📒 Files selected for processing (9)
code/addons/docs/src/mdx-plugin.tscode/builders/builder-vite/src/plugins/inject-export-order-plugin.tscode/builders/builder-vite/src/plugins/storybook-external-globals-plugin.tscode/builders/builder-vite/src/plugins/strip-story-hmr-boundaries.tscode/builders/builder-vite/src/plugins/vite-inject-mocker/plugin.tscode/builders/builder-vite/src/plugins/vite-mock/plugin.tscode/frameworks/svelte-vite/src/plugins/svelte-docgen.tscode/frameworks/vue3-vite/src/plugins/vue-component-meta.tscode/frameworks/vue3-vite/src/plugins/vue-docgen.ts
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/frameworks/vue3-vite/src/plugins/vue-component-meta.ts`:
- Around line 110-112: Remove the duplicated inline comment so only a single
comment remains; locate the consecutive lines containing "// if there is no
component meta, return undefined" in the vue-component-meta code (around the
function that checks/returns component meta, e.g., the get/resolve component
meta helper) and delete the redundant copy, keeping one clear comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28787cea-a115-4a3a-8256-89284f8a8f6e
📒 Files selected for processing (2)
code/frameworks/vue3-vite/src/plugins/vue-component-meta.tscode/frameworks/vue3-vite/src/plugins/vue-docgen.ts
reopening closed #33900 as changes was started on the wrong branch.
Closes #33899
What I did
This PR moves Vite plugins to use
HookObjects instead of functions.For example
transform(code, id)would move toThis allows to reduce the overhead between JS/Rust runtimes for vite 8 with rolldown. Implementation within handlers doesn't change,
filter()are left within to keep plugins backward compatible with vite < 6.3Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
We need to test all affected vite plugin. Just to ensure they run correctly
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit